contracts/mocks/docs/account/paymaster/PaymasterECDSASigner.sol (1)

11-11: Consider separating signer and owner roles — or add an inline comment clarifying the coupling.

Ownable(signerAddr) makes the ECDSA signing key the Ownable owner, so the same address that authorizes user operations also controls fund withdrawals. For a docs/mock contract this is a deliberate simplification, but it could mislead readers into treating the signer and the withdrawal-authorized owner as inherently the same role in production.

If this is intended as an illustrative shortcut, a brief inline comment would improve clarity. Alternatively, the constructor could accept a distinct initialOwner parameter to better reflect the general case:

♻️ Proposed refactor to separate roles

-    constructor(address signerAddr) EIP712("MyPaymasterECDSASigner", "1") Ownable(signerAddr) SignerECDSA(signerAddr) {}
+    constructor(address initialOwner, address signerAddr) EIP712("MyPaymasterECDSASigner", "1") Ownable(initialOwner) SignerECDSA(signerAddr) {}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/mocks/docs/account/paymaster/PaymasterECDSASigner.sol` at line 11,
The constructor currently couples the ECDSA signer and contract owner by calling
Ownable(signerAddr) in PaymasterECDSASigner, which can mislead readers; either
add a brief inline comment next to the constructor/Ownable(signerAddr) call
stating this is an intentional mock simplification and that signer and
withdrawal-owner are intentionally the same here, or change the constructor
signature of PaymasterECDSASigner to accept a separate initialOwner parameter
and initialize Ownable(initialOwner) while keeping SignerECDSA(signerAddr) and
EIP712(...) unchanged so the signing key and owner roles are clearly separated.

test/account/paymaster/Paymaster.behavior.js (1)

191-197: Prefer value over hardcoded 42n in expected stake assertions.

Using the shared constant here keeps the test resilient if the fixture amount changes.

♻️ Suggested tweak

       await expect(predeploy.entrypoint.v09.getDepositInfo(this.paymaster)).to.eventually.deep.equal([
         0n,
         true,
-        42n,
+        value,
         delay,
         0n,
       ]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/account/paymaster/Paymaster.behavior.js` around lines 191 - 197, The
assertion uses a hardcoded stake amount 42n; replace that literal with the
shared fixture constant (value) so the expectation stays in sync with the
fixture—update the expected array in the getDepositInfo assertion
(predeploy.entrypoint.v09.getDepositInfo(this.paymaster)) to use value instead
of 42n.

contracts/account/paymaster/PaymasterERC20.sol (1)

162-164: Add a guard for actualAmount_ > prefundAmount before subtraction.

Line 163 can panic on underflow if estimates drift. Returning (false, actualAmount_) preserves the intended PaymasterERC20FailedRefund path in _postOp.

Suggested hardening

     uint256 actualAmount_ = _erc20Cost(actualGasCost, actualUserOpFeePerGas, tokenPrice);
+    if (actualAmount_ > prefundAmount) {
+        return (false, actualAmount_);
+    }
     return (token.trySafeTransfer(prefunder, prefundAmount - actualAmount_), actualAmount_);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/account/paymaster/PaymasterERC20.sol` around lines 162 - 164, The
subtraction prefundAmount - actualAmount_ can underflow if actualAmount_ >
prefundAmount; in the function that computes actualAmount_ via _erc20Cost and
attempts the refund with token.trySafeTransfer(prefunder, prefundAmount -
actualAmount_), add a guard: if actualAmount_ > prefundAmount then skip the
transfer and return (false, actualAmount_) so the PaymasterERC20FailedRefund
path in _postOp is preserved; otherwise perform the trySafeTransfer as before.
Reference _erc20Cost, actualAmount_, prefundAmount, token.trySafeTransfer and
the PaymasterERC20FailedRefund handling in _postOp when adding the check and
early return.

contracts/account/paymaster/PaymasterERC20Guarantor.sol (1)

67-106: Decode sender from the tail of prefundContext and pass upstream context unchanged.

Line 67 appends userOp.sender, but Line 88 reads from the beginning and Line 105 forwards the augmented context directly to super._refund. This makes extension layering brittle if parent/child contexts are added later.

Suggested refactor

-        address userOpSender = address(bytes20(prefundContext[0x00:0x14]));
+        if (prefundContext.length < 0x14) {
+            return
+                super._refund(
+                    token,
+                    tokenPrice,
+                    actualGasCost,
+                    actualUserOpFeePerGas,
+                    prefunder,
+                    prefundAmount,
+                    prefundContext
+                );
+        }
+
+        uint256 senderOffset = prefundContext.length - 0x14;
+        address userOpSender = address(bytes20(prefundContext[senderOffset:]));
+        bytes calldata upstreamPrefundContext = prefundContext[:senderOffset];
@@
-                prefundContext
+                upstreamPrefundContext
             );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/account/paymaster/PaymasterERC20Guarantor.sol` around lines 67 -
106, The code currently decodes userOp.sender from the start of prefundContext
and forwards the augmented prefundContext to super._refund, making layering
brittle; change _refund to extract the sender from the tail (last 20 bytes) and
pass the original upstream context (prefundContext without the trailing 20-byte
sender) to super._refund. Concretely, in function _refund compute userOpSender =
address(bytes20(prefundContext[prefundContext.length - 20 :
prefundContext.length])), set upstreamContext = prefundContext[0 :
prefundContext.length - 20] (and revert or handle if prefundContext.length <
20), use userOpSender for the transfer logic, and forward upstreamContext (not
the augmented prefundContext) when calling super._refund so parent contracts
receive the unmodified context.

test/account/paymaster/PaymasterERC20.test.js (1)

245-288: Add no-ether-change checks to the revert scenarios.

For these FailedOp tests, please also assert that ether balances do not change for the relevant actors. This makes the revert-path expectations complete and prevents silent side-effect regressions.

Based on learnings: In tests that exercise revert semantics, use negative balance assertions to verify no ether balance changes occurred.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/account/paymaster/PaymasterERC20.test.js` around lines 245 - 288, Add
explicit no-ether-change assertions to each revert test ('reverts with an
invalid token', 'reverts with insufficient balance', 'reverts with insufficient
approval'): record or assert Ether balances for the relevant actors (at minimum
this.account, this.paymaster and this.receiver) before calling
predeploy.entrypoint.v09.handleOps(signedUserOp.packed, this.receiver) and
assert they do not change after the call (e.g. assert zero delta with Chai's
changeEtherBalances or by comparing balances before/after). Update the three
tests so the revert expectation is accompanied by these no-balance-change checks
around predeploy.entrypoint.v09.handleOps to prevent silent ether side-effects.

test/account/paymaster/PaymasterERC20Guarantor.test.js (1)

344-390: Add explicit no-ether-movement assertions for the revert-path cases.

These three tests validate the revert reason, but they don’t verify that balances are unchanged after revert. Please add a no-balance-change assertion for the EntryPoint/beneficiary pair to guard against unintended side effects.

Based on learnings: In tests that exercise revert semantics, use negative balance assertions to verify no ether balance changes occurred.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/account/paymaster/PaymasterERC20Guarantor.test.js` around lines 344 -
390, After each revert assertion, capture and compare balances before and after
the call to ensure no ether moved: read balances with ethers.provider.getBalance
for predeploy.entrypoint.v09.address and this.receiver (e.g., const beforeEP =
await ethers.provider.getBalance(predeploy.entrypoint.v09.address); const
beforeR = await ethers.provider.getBalance(this.receiver);), perform the
existing expect(...).to.be.revertedWithCustomError(...) call, then re-read
balances and assert they are unchanged with expect(afterEP).to.equal(beforeEP)
and expect(afterR).to.equal(beforeR). Do this for the three tests that use
predeploy.entrypoint.v09.handleOps to validate the EntryPoint/beneficiary pair
has zero net ether change.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro